Re: [Qemu-devel] [PATCH qemu v2 2/4] vfio/spapr: Rename local systempagesize variable

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2763 bytes --]

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))));
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]