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

[Oleksandr <olekstysh@gmail.com>]

On 21.08.20 09:16, Jan Beulich wrote:

Hi Jan.

Thank you for your answer.

> On 20.08.2020 20:30, Oleksandr wrote:
>> On 06.08.20 14:29, Jan Beulich wrote:
>>> On 06.08.2020 13:08, Julien Grall wrote:
>>>> On 05/08/2020 20:30, Oleksandr wrote:
>>>>> 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?
>>> Due to the split brain model of handling PCI pass-through (between
>>> Xen and qemu), a guest writing to an MSI-X entry needs this write
>>> handed to qemu, and upon completion of the write there Xen also
>>> needs to take some extra action.
>>
>> 1. Regarding common handle_hvm_io_completion() implementation:
>>
>> Could msix_write_completion() be called later on so we would be able to
>> split handle_hvm_io_completion() gracefully or could we call it from
>> handle_mmio()?
>> The reason I am asking is to avoid calling it from the common code in
>> order to avoid introducing stub on Arm which is not going to be ever
>> implemented
>> (if msix_write_completion() is purely x86 material).
> I'm unconvinced of this last fact, but as with about everything it is
> quite certainly possible to call the function later. The question is
> how ugly this would become, as this may involve redundant conditionals
> (i.e. ones which need to remain in sync) and/or extra propagation of
> state.


I understand. Would it be better to make handle_hvm_io_completion() per 
arch then?
This would avoid using various stubs on Arm (we could get rid of 
has_vpci, msix_write_completion, handle_pio, 
arch_handle_hvm_io_completion, etc)
and avoid renaming of handle_mmio().

Julien what is your opinion on that?


For example the Arm implementation would look like:

bool handle_hvm_io_completion(struct vcpu *v)
{
     struct domain *d = v->domain;
     struct hvm_vcpu_io *vio = &v->arch.hvm.hvm_io;
     struct hvm_ioreq_server *s;
     struct hvm_ioreq_vcpu *sv;
     enum hvm_io_completion io_completion;

     sv = get_pending_vcpu(v, &s);
     if ( sv && !hvm_wait_for_io(sv, get_ioreq(s, v)) )
         return false;

     vio->io_req.state = hvm_ioreq_needs_completion(&vio->io_req) ?
         STATE_IORESP_READY : STATE_IOREQ_NONE;

     vcpu_end_shutdown_deferral(v);

     io_completion = vio->io_completion;
     vio->io_completion = HVMIO_no_completion;

     switch ( io_completion )
     {
     case HVMIO_no_completion:
         break;

     case HVMIO_mmio_completion:
         return ioreq_handle_complete_mmio();

     default:
         ASSERT_UNREACHABLE();
         break;
     }

     return true;
}


>>
>> 2. Regarding renaming common handle_mmio() to ioreq_handle_complete_mmio():
>>
>> There was a request to consider renaming that function which is called
>> from the common code in the context of IOREQ series.
>> The point is, that the name of the function is pretty generic and can be
>> confusing on Arm (we already have a try_handle_mmio()).
>> I noticed that except common code that function is called from a few
>> places on x86 (I am not even sure whether all of them are IOREQ related).
>> The question is would x86 folks be happy with such renaming?
> handle_mmio() without any parameters and used for a varying set
> of purposes was imo never a good choice of name. The situation
> has improved, but can do with further improvement. The new name,
> if to be used for truly renaming the function need to fit all
> uses though. As such, I don't think ioreq_handle_complete_mmio()
> is an appropriate name.
>
>> Alternatively I could provide the following in
>> include/asm-arm/hvm/ioreq.h without renaming it in the common code and
>> still using non-confusing variant on Arm (however I am not sure whether
>> this is a good idea):
>>
>> #define handle_mmio ioreq_handle_complete_mmio
> If anything, for x86 it ought to be the other way around, at
> which point you wouldn't need any alias #define on Arm.
But could this approach be accepted? I think it would be the easiest way
to avoid confusing on Arm and avoid renaming that function in the whole 
x86 code.


>
>> 3. Regarding common IOREQ/DM stuff location:
>>
>> Currently it is located at:
>> common/hvm/...
>> include/xen/hvm/...
>>
>> For the non-RFC patch series I am going to avoid using "hvm" name (which
>> is internal detail of arch specific code and shouldn't be exposed to the
>> common code).
>> The question is whether I should use another directory name (probably
>> ioreq?) or just place them in common root directory?
> I think there are arguments for and against hvm/. I'm not of
> the opinion that ioreq/ would be a good name, so if hvm/ was to
> be ruled out, I think the file(s) shouldn't go into separate
> subdirs at all.

Got it.


-- 
Regards,

Oleksandr Tyshchenko