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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 24.11.21 14:32, Roger Pau Monné wrote:
> On Tue, Nov 23, 2021 at 03:14:27PM +0000, Oleksandr Andrushchenko wrote:
>> 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
> I think guest_reg would be fine?
>
> Or alternatively you could make it a guest address by dropping the low
> bits and adding them in the read handler instead of doing it in the
> write handler.
So, let it be "guest_reg" then
>
> Thanks, Roger.
Thank you,
Oleksandr