Re: [RFC PATCH V1 04/12] xen/arm: Introduce arch specific bits for IOREQ/DM features

[Oleksandr <olekstysh@gmail.com>]

On 06.08.20 14:08, Julien Grall wrote:

Hi Julien

>
>>> What is this function supposed to do?
>> Agree, sounds confusing a bit. I assume it is supposed to complete 
>> Guest MMIO access after finishing emulation.
>>
>> Shall I rename it to something appropriate (maybe by adding ioreq 
>> prefix)?
>
> How about ioreq_handle_complete_mmio()?

For me it sounds fine.



>
>>>> diff --git a/xen/common/memory.c b/xen/common/memory.c
>>>> index 9283e5e..0000477 100644
>>>> --- a/xen/common/memory.c
>>>> +++ b/xen/common/memory.c
>>>> @@ -8,6 +8,7 @@
>>>>    */
>>>>     #include <xen/domain_page.h>
>>>> +#include <xen/hvm/ioreq.h>
>>>>   #include <xen/types.h>
>>>>   #include <xen/lib.h>
>>>>   #include <xen/mm.h>
>>>> @@ -30,10 +31,6 @@
>>>>   #include <public/memory.h>
>>>>   #include <xsm/xsm.h>
>>>>   -#ifdef CONFIG_IOREQ_SERVER
>>>> -#include <xen/hvm/ioreq.h>
>>>> -#endif
>>>> -
>>>
>>> Why do you remove something your just introduced?
>> The reason I guarded that header is to make "xen/mm: Make x86's 
>> XENMEM_resource_ioreq_server handling common" (previous) patch 
>> buildable on Arm
>> without arch IOREQ header added yet. I tried to make sure that the 
>> result after each patch was buildable to retain bisectability.
>> As current patch adds Arm IOREQ specific bits (including header), 
>> that guard could be removed as not needed anymore.
> I agree we want to have the build bisectable. However, I am still 
> puzzled why it is necessary to remove the #ifdef and move it earlier 
> in the list.
>
> Do you mind to provide more details?
Previous patch "xen/mm: Make x86's XENMEM_resource_ioreq_server handling 
common" breaks build on Arm as it includes xen/hvm/ioreq.h which 
requires arch header
to be present (asm/hvm/ioreq.h). But the missing arch header together 
with other arch specific bits are introduced here in current patch. 
Probably I should have rearranged
changes in a way to not introduce #ifdef and then remove it...


>
> [...]
>
>>>> +
>>>> +bool handle_mmio(void);
>>>> +
>>>> +static inline bool handle_pio(uint16_t port, unsigned int size, 
>>>> int dir)
>>>> +{
>>>> +    /* XXX */
>>>
>>> Can you expand this TODO? What do you expect to do?
>> I didn't expect this to be called on Arm. Sorry, I am not sure l have 
>> an idea how to handle this properly. I would keep it unimplemented 
>> until a real reason.
>> Will expand TODO.
>
> Let see how the conversation on patch#1 goes about PIO vs MMIO.

ok


>
>>>
>>>
>>>> +    BUG();
>>>> +    return true;
>>>> +}
>>>> +
>>>> +static inline paddr_t hvm_mmio_first_byte(const ioreq_t *p)
>>>> +{
>>>> +    return p->addr;
>>>> +}
>>>
>>> I understand that the x86 version is more complex as it check p->df. 
>>> However, aside reducing the complexity, I am not sure why we would 
>>> want to diverge it.
>>>
>>> After all, IOREQ is now meant to be a common feature.
>> Well, no objections at all.
>> Could you please clarify how could 'df' (Direction Flag?) be 
>> handled/used on Arm?
>
> On x86, this is used by 'rep' instruction to tell the direction to 
> iterate (forward or backward).
>
> On Arm, all the accesses to MMIO region will do a single memory 
> access. So for now, we can safely always set to 0.
>
>> I see that try_fwd_ioserv() always sets it 0. Or I need to just reuse 
>> x86's helpers as is,
>> which (together with count = df = 0) will result in what we actually 
>> have here?
> AFAIU, both count and df should be 0 on Arm.

Thanks for the explanation. The only one question remains where to put 
common helpers hvm_mmio_first_byte/hvm_mmio_last_byte (common io.h?)?


>>>
>>>> +
>>>> +static inline int p2m_set_ioreq_server(struct domain *d,
>>>> +                                       unsigned int flags,
>>>> +                                       struct hvm_ioreq_server *s)
>>>> +{
>>>> +    return -EOPNOTSUPP;
>>>> +}
>>>
>>> This should be defined in p2m.h. But I am not even sure what it is 
>>> meant for. Can you expand it?
>>
>> ok, will move.
>>
>>
>> In this series I tried to make as much IOREQ code common as possible 
>> and avoid complicating things, in order to achieve that a few stubs 
>> were added here. Please note,
>> that I also considered splitting into arch parts. But some functions 
>> couldn't be split easily.
>> This one is called from common hvm_destroy_ioreq_server() with flag 
>> being 0 (which will result in unmapping ioreq server from p2m type on 
>> x86).
>> I could add a comment describing why this stub is present here.
>
> Sorry if I wasn't clear. I wasn't asking why the stub is there but 
> what should be the expected implementation of the function.
>
> In particular, you are returning -EOPNOTSUPP. The only reason you are 
> getting away from trouble is because the caller doesn't check the return.

True.


>
> Would it make sense to have a stub arch_hvm_destroy_ioreq_server()?

With what has been said above, it make sense, will create.


>>>> +
>>>> +static inline void msix_write_completion(struct vcpu *v)
>>>> +{
>>>> +}
>>>> +
>>>> +static inline void handle_realmode_completion(void)
>>>> +{
>>>> +    ASSERT_UNREACHABLE();
>>>> +}
>>>
>>> realmode is very x86 specific. So I don't think this function should 
>>> be called from common code. It might be worth considering to split 
>>> handle_hvm_io_completion() is 2 parts: common and arch specific.
>>
>> I agree with you that realmode is x86 specific and looks not good in 
>> Arm header. 
> It is not a problem of looking good or not. Instead, it is about 
> abstraction. A developper shouldn't need to understand all the other 
> architectures we support in order to follow the common code.
>
>> I was thinking how to split handle_hvm_io_completion() gracefully but 
>> I failed find a good solution for that, so decided to add two stubs 
>> (msix_write_completion and handle_realmode_completion) on Arm. I 
>> could add a comment describing why they are here if appropriate. But 
>> if you think they shouldn't be called from the common code in any 
>> way, I will try to split it.
>
> I am not entirely sure what msix_write_completion is meant to do on 
> x86. Is it dealing with virtual MSIx? Maybe Jan, Roger or Paul could 
> help?
>
> Regarding handle_realmode_completion, I would add a new stub:
>
> arch_ioreq_handle_io_completion() that is called from the default case 
> of the switch.
>
> On x86 it would be implemented as:
>
>  switch (io_completion)
>  {
>     case HVMIO_realmode_completion:
>       ...
>     default:
>       ASSERT_UNREACHABLE();
>  }
>
> On Arm, it would be implemented as:
>
>   ASSERT_UNREACHABLE();


Good point, will update.


-- 
Regards,

Oleksandr Tyshchenko