Re: [PATCH v5 06/14] vpci/header: implement guest BAR register handlers

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 01.02.22 12:10, Roger Pau Monné wrote:
> On Tue, Feb 01, 2022 at 07:31:31AM +0000, Oleksandr Andrushchenko wrote:
>>
>> On 31.01.22 17:50, Jan Beulich wrote:
>>> On 31.01.2022 16:06, Oleksandr Andrushchenko wrote:
>>>> Hi, Roger!
>>>>>>                 rom->type = VPCI_BAR_EMPTY;
>>>>>>         }
>>>>>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>>>>>> index ed127a08a953..0a73b14a92dc 100644
>>>>>> --- a/xen/include/xen/vpci.h
>>>>>> +++ b/xen/include/xen/vpci.h
>>>>>> @@ -68,7 +68,10 @@ struct vpci {
>>>>>>         struct vpci_header {
>>>>>>             /* Information about the PCI BARs of this device. */
>>>>>>             struct vpci_bar {
>>>>>> +            /* Physical view of the BAR. */
>>>>> No, that's not the physical view, it's the physical (host) address.
>>>>>
>>>>>>                 uint64_t addr;
>>>>>> +            /* Guest view of the BAR: address and lower bits. */
>>>>>> +            uint64_t guest_reg;
>>>>> I continue to think it would be clearer if you store the guest address
>>>>> here (gaddr, without the low bits) and add those in guest_bar_read
>>>>> based on bar->{type,prefetchable}. Then it would be equivalent to the
>>>>> existing 'addr' field.
>>>>>
>>>> I agreed first to do such a change, but then recalled our discussion with 
>>>> Jan [1].
>>>> And then we decided that in order for it to be efficient it is better if 
>>>> we setup all the
>>>> things during the write phase (rare), rather then during the write phase 
>>>> (more often).
>>> Small correction: The 2nd "write" was likely meant to be "read".
>> Yes, this is correct.
>>>    But
>>> please recall that Roger is the maintainer of the code, so he gets
>>> the final say.
>> Agree, but would vote for the current approach as it still saves some
>> CPU cycles making the read operation really tiny
> I think you need to build the mapping rangeset(s) based on guest
> addresses, not host ones, so it's likely going to be easier if you
> store the address here in order to use it when building the rangeset.
>
> Overall the cost of the vmexit will shadow the cost of doing a couple
> of ORs here in order to return the guest view of the BAR.
>
> If you think storing the guest view of the BAR register will make the
> code easier to understand, then please go ahead. Otherwise I would
> recommend to store the address like we do for the host position of the
> BAR (ie: addr field).
I still think it is easier to understand: if you take a look at what we do
for BAR write for both host and guest you'll see that we do almost the
same operations, but in host case we end up writing bar->addr + low
bits to the HW register and in case of a guest we store the complete
thing into bar->guest_reg. Read operation doesn't require any processing
for host, so it is equivalent to direct hw read and in case of a guest it
is as simple as possible and implements the equivalent by returning
part of bar->guest_reg (hi or lo).  So, from this POV it is IMO easier to
understand the logic.
That being said, I do agree that the contents of the bar->addr is not
equivalent to bar->guest_reg, but we have already taken care of it
by naming the guest's one with guest_reg, not guest_addr.

I will keep the code as is then.
>
> Thanks, Roger.
Thank you,
Oleksandr