Re: [PATCH v4 05/11] vpci/header: implement guest BAR register handlers

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 19.11.21 15:02, Jan Beulich wrote:
> On 19.11.2021 13:54, Oleksandr Andrushchenko wrote:
>> On 19.11.21 14:49, Jan Beulich wrote:
>>> On 19.11.2021 13:46, Oleksandr Andrushchenko wrote:
>>>> On 19.11.21 14:37, Jan Beulich wrote:
>>>>> On 19.11.2021 13:10, Oleksandr Andrushchenko wrote:
>>>>>> On 19.11.21 13:58, Jan Beulich wrote:
>>>>>>> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
>>>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>>>> @@ -408,6 +408,48 @@ static void bar_write(const struct pci_dev *pdev, 
>>>>>>>> unsigned int reg,
>>>>>>>>          pci_conf_write32(pdev->sbdf, reg, val);
>>>>>>>>      }
>>>>>>>>      
>>>>>>>> +static void guest_bar_write(const struct pci_dev *pdev, unsigned int 
>>>>>>>> reg,
>>>>>>>> +                            uint32_t val, void *data)
>>>>>>>> +{
>>>>>>>> +    struct vpci_bar *bar = data;
>>>>>>>> +    bool hi = false;
>>>>>>>> +
>>>>>>>> +    if ( bar->type == VPCI_BAR_MEM64_HI )
>>>>>>>> +    {
>>>>>>>> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
>>>>>>>> +        bar--;
>>>>>>>> +        hi = true;
>>>>>>>> +    }
>>>>>>>> +    else
>>>>>>>> +    {
>>>>>>>> +        val &= PCI_BASE_ADDRESS_MEM_MASK;
>>>>>>>> +        val |= bar->type == VPCI_BAR_MEM32 ? 
>>>>>>>> PCI_BASE_ADDRESS_MEM_TYPE_32
>>>>>>>> +                                           : 
>>>>>>>> PCI_BASE_ADDRESS_MEM_TYPE_64;
>>>>>>>> +        val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 0;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
>>>>>>>> +    bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);
>>>>>>>> +
>>>>>>>> +    bar->guest_addr &= ~(bar->size - 1) | ~PCI_BASE_ADDRESS_MEM_MASK;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned 
>>>>>>>> int reg,
>>>>>>>> +                               void *data)
>>>>>>>> +{
>>>>>>>> +    const struct vpci_bar *bar = data;
>>>>>>>> +    bool hi = false;
>>>>>>>> +
>>>>>>>> +    if ( bar->type == VPCI_BAR_MEM64_HI )
>>>>>>>> +    {
>>>>>>>> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
>>>>>>>> +        bar--;
>>>>>>>> +        hi = true;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    return bar->guest_addr >> (hi ? 32 : 0);
>>>>>>> I'm afraid "guest_addr" then isn't the best name; maybe "guest_val"?
>>>>>>> This would make more obvious that there is a meaningful difference
>>>>>>> from "addr" besides the guest vs host aspect.
>>>>>> I am not sure I can agree here:
>>>>>> bar->addr and bar->guest_addr make it clear what are these while
>>>>>> bar->addr and bar->guest_val would make someone go look for
>>>>>> additional information about what that val is for.
>>>>> Feel free to replace "val" with something more suitable. "guest_bar"
>>>>> maybe? The value definitely is not an address, so "addr" seems
>>>>> inappropriate / misleading to me.
>>>> This is a guest's view on the BAR's address. So to me it is still 
>>>> guest_addr
>>> It's a guest's view on the BAR, not just the address. Or else you couldn't
>>> simply return the value here without folding in the correct low bits.
>> I agree with this this respect as it is indeed address + lower bits.
>> How about guest_bar_val then? So it reflects its nature, e.g. the value
>> of the BAR as seen by the guest.
> Gets a little longish for my taste. I for one wouldn't mind it be just
> "guest". In the end Roger has the final say here anyway.
What is your preference on naming here?
1. guest_addr
2. guest_val
3. guest_bar_val
4. guest
>
> Jan
>
Thank you in advance,
Oleksandr