| Message ID | 20190214052144.59541-3-aik@ozlabs.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr_pci, vfio: NVIDIA V100 + POWER9 passthrough | expand |
On Thu, Feb 14, 2019 at 04:21:42PM +1100, Alexey Kardashevskiy wrote: > The "systempagesize" name suggests that it is the host system page size > while it is the smallest page size of memory backing the guest RAM so > let's rename it to stop confusion. This should cause no behavioral change. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > hw/vfio/spapr.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c > index 302d6b0..1498fee 100644 > --- a/hw/vfio/spapr.c > +++ b/hw/vfio/spapr.c > @@ -148,14 +148,14 @@ int vfio_spapr_create_window(VFIOContainer *container, > uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr); > unsigned entries, bits_total, bits_per_level, max_levels; > struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) }; > - long systempagesize = qemu_getrampagesize(); > + long minpagesize = qemu_getrampagesize(); I'd prefer "rampagesize" or "minrampagesize" here. "minpagesize" doesn't make it clear what it's the minimum of, whereas rampagesize at least makes it clear it's the same thing that qemu_getrampagesize() is working with. > /* > * The host might not support the guest supported IOMMU page size, > * so we will use smaller physical IOMMU pages to back them. > */ > - if (pagesize > systempagesize) { > - pagesize = systempagesize; > + if (pagesize > minpagesize) { > + pagesize = minpagesize; > } > pagesize = 1ULL << (63 - clz64(container->pgsizes & > (pagesize | (pagesize - 1))));
On 15/02/2019 14:32, David Gibson wrote: > On Thu, Feb 14, 2019 at 04:21:42PM +1100, Alexey Kardashevskiy wrote: >> The "systempagesize" name suggests that it is the host system page size >> while it is the smallest page size of memory backing the guest RAM so >> let's rename it to stop confusion. This should cause no behavioral change. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> hw/vfio/spapr.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c >> index 302d6b0..1498fee 100644 >> --- a/hw/vfio/spapr.c >> +++ b/hw/vfio/spapr.c >> @@ -148,14 +148,14 @@ int vfio_spapr_create_window(VFIOContainer *container, >> uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr); >> unsigned entries, bits_total, bits_per_level, max_levels; >> struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) }; >> - long systempagesize = qemu_getrampagesize(); >> + long minpagesize = qemu_getrampagesize(); > > I'd prefer "rampagesize" or "minrampagesize" here. "minpagesize" > doesn't make it clear what it's the minimum of, whereas rampagesize at > least makes it clear it's the same thing that qemu_getrampagesize() is > working with. In my mind "ram" in this context is a guest RAM but here is it not (is is backend) so I just avoided using any specifics in the name (system, ram) to make the reader of this code look around where the value comes from which is the qemu_getrampagesize() helper (more descriptive name). > >> /* >> * The host might not support the guest supported IOMMU page size, >> * so we will use smaller physical IOMMU pages to back them. >> */ >> - if (pagesize > systempagesize) { >> - pagesize = systempagesize; >> + if (pagesize > minpagesize) { >> + pagesize = minpagesize; >> } >> pagesize = 1ULL << (63 - clz64(container->pgsizes & >> (pagesize | (pagesize - 1)))); >
On Fri, Feb 15, 2019 at 02:37:55PM +1100, Alexey Kardashevskiy wrote: > > > On 15/02/2019 14:32, David Gibson wrote: > > On Thu, Feb 14, 2019 at 04:21:42PM +1100, Alexey Kardashevskiy wrote: > >> The "systempagesize" name suggests that it is the host system page size > >> while it is the smallest page size of memory backing the guest RAM so > >> let's rename it to stop confusion. This should cause no behavioral change. > >> > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > >> --- > >> hw/vfio/spapr.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c > >> index 302d6b0..1498fee 100644 > >> --- a/hw/vfio/spapr.c > >> +++ b/hw/vfio/spapr.c > >> @@ -148,14 +148,14 @@ int vfio_spapr_create_window(VFIOContainer *container, > >> uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr); > >> unsigned entries, bits_total, bits_per_level, max_levels; > >> struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) }; > >> - long systempagesize = qemu_getrampagesize(); > >> + long minpagesize = qemu_getrampagesize(); > > > > I'd prefer "rampagesize" or "minrampagesize" here. "minpagesize" > > doesn't make it clear what it's the minimum of, whereas rampagesize at > > least makes it clear it's the same thing that qemu_getrampagesize() is > > working with. > > In my mind "ram" in this context is a guest RAM but here is it not (is > is backend) Well.. it is guest RAM, in that we're only considering the backing page size for memory mapped into the guest as RAM. I guess it is a host page size, but then guest page sizes don't really exist until the guest starts mapping things (and may not be unique, since it could map the same memory in multiple ways). > so I just avoided using any specifics in the name (system, > ram) to make the reader of this code look around where the value comes > from which is the qemu_getrampagesize() helper (more descriptive name). > > > > > > >> /* > >> * The host might not support the guest supported IOMMU page size, > >> * so we will use smaller physical IOMMU pages to back them. > >> */ > >> - if (pagesize > systempagesize) { > >> - pagesize = systempagesize; > >> + if (pagesize > minpagesize) { > >> + pagesize = minpagesize; > >> } > >> pagesize = 1ULL << (63 - clz64(container->pgsizes & > >> (pagesize | (pagesize - 1)))); > > >
On 15/02/2019 14:57, David Gibson wrote: > On Fri, Feb 15, 2019 at 02:37:55PM +1100, Alexey Kardashevskiy wrote: >> >> >> On 15/02/2019 14:32, David Gibson wrote: >>> On Thu, Feb 14, 2019 at 04:21:42PM +1100, Alexey Kardashevskiy wrote: >>>> The "systempagesize" name suggests that it is the host system page size >>>> while it is the smallest page size of memory backing the guest RAM so >>>> let's rename it to stop confusion. This should cause no behavioral change. >>>> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> --- >>>> hw/vfio/spapr.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c >>>> index 302d6b0..1498fee 100644 >>>> --- a/hw/vfio/spapr.c >>>> +++ b/hw/vfio/spapr.c >>>> @@ -148,14 +148,14 @@ int vfio_spapr_create_window(VFIOContainer *container, >>>> uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr); >>>> unsigned entries, bits_total, bits_per_level, max_levels; >>>> struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) }; >>>> - long systempagesize = qemu_getrampagesize(); >>>> + long minpagesize = qemu_getrampagesize(); >>> >>> I'd prefer "rampagesize" or "minrampagesize" here. "minpagesize" >>> doesn't make it clear what it's the minimum of, whereas rampagesize at >>> least makes it clear it's the same thing that qemu_getrampagesize() is >>> working with. >> >> In my mind "ram" in this context is a guest RAM but here is it not (is >> is backend) > > Well.. it is guest RAM, in that we're only considering the backing > page size for memory mapped into the guest as RAM. I guess it is a > host page size, but then guest page sizes don't really exist until the > guest starts mapping things (and may not be unique, since it could > map the same memory in multiple ways). Well, I'll make rampagesize then. >> so I just avoided using any specifics in the name (system, >> ram) to make the reader of this code look around where the value comes >> from which is the qemu_getrampagesize() helper (more descriptive name). >> >> >> >>> >>>> /* >>>> * The host might not support the guest supported IOMMU page size, >>>> * so we will use smaller physical IOMMU pages to back them. >>>> */ >>>> - if (pagesize > systempagesize) { >>>> - pagesize = systempagesize; >>>> + if (pagesize > minpagesize) { >>>> + pagesize = minpagesize; >>>> } >>>> pagesize = 1ULL << (63 - clz64(container->pgsizes & >>>> (pagesize | (pagesize - 1)))); >>> >> >
diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c index 302d6b0..1498fee 100644 --- a/hw/vfio/spapr.c +++ b/hw/vfio/spapr.c @@ -148,14 +148,14 @@ int vfio_spapr_create_window(VFIOContainer *container, uint64_t pagesize = memory_region_iommu_get_min_page_size(iommu_mr); unsigned entries, bits_total, bits_per_level, max_levels; struct vfio_iommu_spapr_tce_create create = { .argsz = sizeof(create) }; - long systempagesize = qemu_getrampagesize(); + long minpagesize = qemu_getrampagesize(); /* * The host might not support the guest supported IOMMU page size, * so we will use smaller physical IOMMU pages to back them. */ - if (pagesize > systempagesize) { - pagesize = systempagesize; + if (pagesize > minpagesize) { + pagesize = minpagesize; } pagesize = 1ULL << (63 - clz64(container->pgsizes & (pagesize | (pagesize - 1))));
The "systempagesize" name suggests that it is the host system page size while it is the smallest page size of memory backing the guest RAM so let's rename it to stop confusion. This should cause no behavioral change. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- hw/vfio/spapr.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)