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 <a...@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))));
>>>
>>
>
--
Alexey
