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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> For the non-RFC patch series I moved handle_realmode_completion to the 
> x86 code and now my local implementation looks 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;
> 
>      if ( has_vpci(d) && vpci_process_pending(v) )
>      {
>          raise_softirq(SCHEDULE_SOFTIRQ);
>          return false;
>      }
> 
>      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;
> 
>      msix_write_completion(v);
>      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 handle_mmio();
> 
>      case HVMIO_pio_completion:
>          return handle_pio(vio->io_req.addr, vio->io_req.size,
>                            vio->io_req.dir);
> 
>      default:
>          return arch_handle_hvm_io_completion(io_completion);
>      }
> 
>      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.

> 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.

Jan