Re: [PATCH 5/9] vpci/header: Implement guest BAR register handlers

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 08.09.21 13:03, Jan Beulich wrote:
> On 08.09.2021 11:43, Oleksandr Andrushchenko wrote:
>> On 08.09.21 12:27, Jan Beulich wrote:
>>> On 07.09.2021 19:39, Oleksandr Andrushchenko wrote:
>>>> On 07.09.21 19:30, Jan Beulich wrote:
>>>>> On 07.09.2021 15:33, Oleksandr Andrushchenko wrote:
>>>>>> On 06.09.21 17:31, Jan Beulich wrote:
>>>>>>> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>>>>>>>> --- a/xen/drivers/vpci/header.c
>>>>>>>> +++ b/xen/drivers/vpci/header.c
>>>>>>>> @@ -400,12 +400,72 @@ static void bar_write(const struct pci_dev 
>>>>>>>> *pdev, unsigned int reg,
>>>>>>>>      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;
>>>>>>>> +    bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
>>>>>>>> +    bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);
>>>>>>> What you store here is not the address that's going to be used,
>>>>>>>      as
>>>>>>> you don't mask off the low bits (to account for the BAR's size).
>>>>>>> When a BAR gets written with all ones, all writable bits get these
>>>>>>> ones stored. The address of the BAR, aiui, really changes to
>>>>>>> (typically) close below 4Gb (in the case of a 32-bit BAR), which
>>>>>>> is why memory / I/O decoding should be off while sizing BARs.
>>>>>>> Therefore you shouldn't look for the specific "all writable bits
>>>>>>> are ones" pattern (or worse, as you presently do, the "all bits
>>>>>>> outside of the type specifier are ones" one) on the read path.
>>>>>>> Instead mask the value appropriately here, and simply return back
>>>>>>> the stored value from the read path.
>> But in case of BAR sizing I need to actually return BAR size.
>> So, the comparison is the way to tell if the guest wants to read
>> actual (configured) BAR value or it tries to determine BAR's size.
>> This is why I compare and use the result as the answer to what needs
>> to be supplied to the guest. So, if I don't compare with 0xffffffff for the
>> hi part and 0xfffffff0 for the low then how do I know when to return
>> configured BAR or return the size?
> Well, but that's the common misunderstanding that I've been trying
> to point out: There's no difference between these two forms of
> reads. The BARs are simply registers with some r/o bits. There's
> no hidden 2nd register recording what was last written. When you
> write 0xffffffff, all you do is set all writable bits to 1. When
> you read back from the register, you will find all r/o bits
> unchanged (which in particular means all lower address bits are
> zero, thus allowing you to determine the size).
>
> When the spec says to write 0xffffffff for sizing purposes, OSes
> should follow that, yes. This doesn't preclude them to use other
> forms of writes for whichever purpose. Hence you do not want to
> special case sizing, but instead you want to emulate correctly
> all forms of writes, including subsequent reads to uniformly
> return the intended / expected values.
>
> Just to give an example (perhaps a little contrived): To size a
> 64-bit BAR, in principle you'd first need to write 0xffffffff to
> both halves. But there's nothing wrong with doing this in a
> different order: Act on the low half alone first, and then act
> on the high half. The acting on the high half could even be
> skipped if the low half sizing produced at least bit 31 set. Now
> if you were to special case seeing ffffffff:fffffff? as the
> last written pair of values, you'd break that (imo legitimate)
> alternative process of sizing.

How about:

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)
{
     struct vpci_bar *bar = data;

     if ( bar->type == VPCI_BAR_MEM64_HI )
         return bar->guest_addr >> 32;

     return bar->guest_addr;
}

It seems to solve all the questions we have: more work on write path,

no comparison with 0xffffffff: BAR's size is used to mask unwanted bits.

BTW, bars[i].guest_addr = 0; is needed as this field can be re-used.
>
> Jan
>
Thank you,

Oleksandr