From: Oleksandr <olekstysh@gmail.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: xen-devel@lists.xenproject.org,
"Oleksandr Tyshchenko" <oleksandr_tyshchenko@epam.com>,
"Andrew Cooper" <andrew.cooper3@citrix.com>,
"George Dunlap" <george.dunlap@citrix.com>,
"Ian Jackson" <ian.jackson@eu.citrix.com>,
"Julien Grall" <julien@xen.org>,
"Stefano Stabellini" <sstabellini@kernel.org>,
"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
"Paul Durrant" <paul@xen.org>,
"Jun Nakajima" <jun.nakajima@intel.com>,
"Kevin Tian" <kevin.tian@intel.com>, "Tim Deegan" <tim@xen.org>,
"Julien Grall" <julien.grall@arm.com>
Subject: Re: [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common
Date: Wed, 23 Sep 2020 15:28:45 +0300 [thread overview]
Message-ID: <9ebdca87-4105-c27b-635d-7a1b6d4cde82@gmail.com> (raw)
In-Reply-To: <5e59dd52-71ea-6c63-8f63-13928813bb2f@suse.com>
On 22.09.20 18:52, Jan Beulich wrote:
Hi Jan
> On 22.09.2020 17:05, Oleksandr wrote:
>> 2. *arch.hvm.params*: Two functions that use it
>> (hvm_alloc_legacy_ioreq_gfn/hvm_free_legacy_ioreq_gfn) either go into
>> arch code completely or
>> specific macro is used in common code:
>>
>> #define ioreq_get_params(d, i) ((d)->arch.hvm.params[i])
> If Arm has the concept of params, then perhaps. But I didn't think
> Arm does ...
I think it has in some degree, there is a handling of
HVMOP_set_param/HVMOP_get_param and
also there is a code to setup HVM_PARAM_CALLBACK_IRQ.
>
>> I would prefer macro than moving functions to arch code (which are
>> equal and should remain in sync).
> Yes, if the rest of the code is identical, I agree it's better to
> merely abstract away small pieces like this one.
ok
>
>> 3. *arch.hvm.hvm_io*: We could also use the following:
>>
>> #define ioreq_get_io_completion(v) ((v)->arch.hvm.hvm_io.io_completion)
>> #define ioreq_get_io_req(v) ((v)->arch.hvm.hvm_io.io_req)
>>
>> This way struct hvm_vcpu_io won't be used in common code as well.
> But if Arm needs similar field, why keep them in arch.hvm.hvm_io?
Yes, Arm needs the "some" fields, but not "all of them" as x86 has.
For example Arm needs only the following (at least in the context of
this series):
+struct hvm_vcpu_io {
+ /* I/O request in flight to device model. */
+ enum hvm_io_completion io_completion;
+ ioreq_t io_req;
+
+ /*
+ * HVM emulation:
+ * Linear address @mmio_gla maps to MMIO physical frame @mmio_gpfn.
+ * The latter is known to be an MMIO frame (not RAM).
+ * This translation is only valid for accesses as per @mmio_access.
+ */
+ struct npfec mmio_access;
+ unsigned long mmio_gla;
+ unsigned long mmio_gpfn;
+};
But for x86 the number of fields is quite bigger. If they were in same
way applicable for both archs (as what we have with ioreq_server struct)
I would move it to the common domain. I didn't think of a better idea
than just abstracting accesses to these (used in common ioreq.c) two
fields by macro.
>
>> --- a/xen/common/ioreq.c
>> +++ b/xen/common/ioreq.c
>> @@ -194,7 +194,7 @@ static bool hvm_wait_for_io(struct hvm_ioreq_vcpu
>> *sv, ioreq_t *p)
>> bool handle_hvm_io_completion(struct vcpu *v)
>> {
>> struct domain *d = v->domain;
>> - struct hvm_vcpu_io *vio = &v->arch.hvm.hvm_io;
>> + ioreq_t io_req = ioreq_get_io_req(v);
>> struct hvm_ioreq_server *s;
>> struct hvm_ioreq_vcpu *sv;
>> enum hvm_io_completion io_completion;
>> @@ -209,14 +209,14 @@ bool handle_hvm_io_completion(struct vcpu *v)
>> if ( sv && !hvm_wait_for_io(sv, get_ioreq(s, v)) )
>> return false;
>>
>> - vio->io_req.state = hvm_ioreq_needs_completion(&vio->io_req) ?
>> + io_req.state = hvm_ioreq_needs_completion(&io_req) ?
>> STATE_IORESP_READY : STATE_IOREQ_NONE;
> This is unlikely to be correct - you're now updating an on-stack
> copy of the ioreq_t instead of what vio points at.
Oh, thank you for pointing this, I should have used ioreq_t *io_req =
&ioreq_get_io_req(v);
I don't like ioreq_get_io_req much), probably ioreq_req would sound a
little bit better?
>
>> msix_write_completion(v);
>> vcpu_end_shutdown_deferral(v);
>>
>> - io_completion = vio->io_completion;
>> - vio->io_completion = HVMIO_no_completion;
>> + io_completion = ioreq_get_io_completion(v);
>> + ioreq_get_io_completion(v) = HVMIO_no_completion;
> I think it's at least odd to have an lvalue with this kind of a
> name. Perhaps want to drop "get" if it's really meant to be used
> like this.
ok
>
>> @@ -247,7 +247,7 @@ static gfn_t hvm_alloc_legacy_ioreq_gfn(struct
>> hvm_ioreq_server *s)
>> for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )
>> {
>> if ( !test_and_clear_bit(i, &d->ioreq_gfn.legacy_mask) )
>> - return _gfn(d->arch.hvm.params[i]);
>> + return _gfn(ioreq_get_params(d, i));
>> }
>>
>> return INVALID_GFN;
>> @@ -279,7 +279,7 @@ static bool hvm_free_legacy_ioreq_gfn(struct
>> hvm_ioreq_server *s,
>>
>> for ( i = HVM_PARAM_IOREQ_PFN; i <= HVM_PARAM_BUFIOREQ_PFN; i++ )
>> {
>> - if ( gfn_eq(gfn, _gfn(d->arch.hvm.params[i])) )
>> + if ( gfn_eq(gfn, _gfn(ioreq_get_params(d, i))) )
>> break;
>> }
>> if ( i > HVM_PARAM_BUFIOREQ_PFN )
> And these two are needed by Arm? Shouldn't Arm exclusively use
> the new model, via acquire_resource?
I dropped HVMOP plumbing on Arm as it was requested. Only acquire
interface should be used.
This code is not supposed to be called on Arm, but it is a part of
common code and we need to find a way how to abstract away *arch.hvm.params*
Am I correct?
--
Regards,
Oleksandr Tyshchenko
next prev parent reply other threads:[~2020-09-23 12:29 UTC|newest]
Thread overview: 111+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-10 20:21 [PATCH V1 00/16] IOREQ feature (+ virtio-mmio) on Arm Oleksandr Tyshchenko
2020-09-10 20:21 ` [PATCH V1 01/16] x86/ioreq: Prepare IOREQ feature for making it common Oleksandr Tyshchenko
2020-09-14 13:52 ` Jan Beulich
2020-09-21 12:22 ` Oleksandr
2020-09-21 12:31 ` Jan Beulich
2020-09-21 12:47 ` Oleksandr
2020-09-21 13:29 ` Jan Beulich
2020-09-21 14:43 ` Oleksandr
2020-09-21 15:28 ` Jan Beulich
2020-09-23 17:22 ` Julien Grall
2020-09-23 18:08 ` Oleksandr
2020-09-10 20:21 ` [PATCH V1 02/16] xen/ioreq: Make x86's IOREQ feature common Oleksandr Tyshchenko
2020-09-14 14:17 ` Jan Beulich
2020-09-21 19:02 ` Oleksandr
2020-09-22 6:33 ` Jan Beulich
2020-09-22 9:58 ` Oleksandr
2020-09-22 10:54 ` Jan Beulich
2020-09-22 15:05 ` Oleksandr
2020-09-22 15:52 ` Jan Beulich
2020-09-23 12:28 ` Oleksandr [this message]
2020-09-24 10:58 ` Jan Beulich
2020-09-24 15:38 ` Oleksandr
2020-09-24 15:51 ` Jan Beulich
2020-09-24 18:01 ` Julien Grall
2020-09-25 8:19 ` Paul Durrant
2020-09-30 13:39 ` Oleksandr
2020-09-30 17:47 ` Julien Grall
2020-10-01 6:59 ` Paul Durrant
2020-10-01 8:49 ` Jan Beulich
2020-10-01 8:50 ` Paul Durrant
2020-09-10 20:21 ` [PATCH V1 03/16] xen/ioreq: Make x86's hvm_ioreq_needs_completion() common Oleksandr Tyshchenko
2020-09-14 14:59 ` Jan Beulich
2020-09-22 16:16 ` Oleksandr
2020-09-23 17:27 ` Julien Grall
2020-09-10 20:21 ` [PATCH V1 04/16] xen/ioreq: Provide alias for the handle_mmio() Oleksandr Tyshchenko
2020-09-14 15:10 ` Jan Beulich
2020-09-22 16:20 ` Oleksandr
2020-09-23 17:28 ` Julien Grall
2020-09-23 18:17 ` Oleksandr
2020-09-10 20:21 ` [PATCH V1 05/16] xen/ioreq: Make x86's hvm_mmio_first(last)_byte() common Oleksandr Tyshchenko
2020-09-14 15:13 ` Jan Beulich
2020-09-22 16:24 ` Oleksandr
2020-09-10 20:22 ` [PATCH V1 06/16] xen/ioreq: Make x86's hvm_ioreq_(page/vcpu/server) structs common Oleksandr Tyshchenko
2020-09-14 15:16 ` Jan Beulich
2020-09-14 15:59 ` Julien Grall
2020-09-22 16:33 ` Oleksandr
2020-09-10 20:22 ` [PATCH V1 07/16] xen/dm: Make x86's DM feature common Oleksandr Tyshchenko
2020-09-14 15:56 ` Jan Beulich
2020-09-22 16:46 ` Oleksandr
2020-09-24 11:03 ` Jan Beulich
2020-09-24 12:47 ` Oleksandr
2020-09-23 17:35 ` Julien Grall
2020-09-23 18:28 ` Oleksandr
2020-09-10 20:22 ` [PATCH V1 08/16] xen/mm: Make x86's XENMEM_resource_ioreq_server handling common Oleksandr Tyshchenko
2020-09-10 20:22 ` [PATCH V1 09/16] arm/ioreq: Introduce arch specific bits for IOREQ/DM features Oleksandr Tyshchenko
2020-09-11 10:14 ` Oleksandr
2020-09-16 7:51 ` Jan Beulich
2020-09-22 17:12 ` Oleksandr
2020-09-23 18:03 ` Julien Grall
2020-09-23 20:16 ` Oleksandr
2020-09-24 11:08 ` Jan Beulich
2020-09-24 16:02 ` Oleksandr
2020-09-24 18:02 ` Oleksandr
2020-09-25 6:51 ` Jan Beulich
2020-09-25 9:47 ` Oleksandr
2020-09-26 13:12 ` Julien Grall
2020-09-26 13:18 ` Oleksandr
2020-09-24 16:51 ` Julien Grall
2020-09-24 17:25 ` Julien Grall
2020-09-24 18:22 ` Oleksandr
2020-09-26 13:21 ` Julien Grall
2020-09-26 14:57 ` Oleksandr
2020-09-10 20:22 ` [PATCH V1 10/16] xen/mm: Handle properly reference in set_foreign_p2m_entry() on Arm Oleksandr Tyshchenko
2020-09-16 7:17 ` Jan Beulich
2020-09-16 8:50 ` Julien Grall
2020-09-16 8:52 ` Jan Beulich
2020-09-16 8:55 ` Julien Grall
2020-09-22 17:30 ` Oleksandr
2020-09-16 8:08 ` Jan Beulich
2020-09-10 20:22 ` [PATCH V1 11/16] xen/ioreq: Introduce hvm_domain_has_ioreq_server() Oleksandr Tyshchenko
2020-09-16 8:04 ` Jan Beulich
2020-09-16 8:13 ` Paul Durrant
2020-09-16 8:39 ` Julien Grall
2020-09-16 8:43 ` Paul Durrant
2020-09-22 18:39 ` Oleksandr
2020-09-22 18:23 ` Oleksandr
2020-09-10 20:22 ` [PATCH V1 12/16] xen/dm: Introduce xendevicemodel_set_irq_level DM op Oleksandr Tyshchenko
2020-09-26 13:50 ` Julien Grall
2020-09-26 14:21 ` Oleksandr
2020-09-10 20:22 ` [PATCH V1 13/16] xen/ioreq: Make x86's invalidate qemu mapcache handling common Oleksandr Tyshchenko
2020-09-16 8:50 ` Jan Beulich
2020-09-22 19:32 ` Oleksandr
2020-09-24 11:16 ` Jan Beulich
2020-09-24 16:45 ` Oleksandr
2020-09-25 7:03 ` Jan Beulich
2020-09-25 13:05 ` Oleksandr
2020-10-02 9:55 ` Oleksandr
2020-10-07 10:38 ` Julien Grall
2020-10-07 12:01 ` Oleksandr
2020-09-10 20:22 ` [PATCH V1 14/16] xen/ioreq: Use guest_cmpxchg64() instead of cmpxchg() Oleksandr Tyshchenko
2020-09-16 9:04 ` Jan Beulich
2020-09-16 9:07 ` Julien Grall
2020-09-16 9:09 ` Paul Durrant
2020-09-16 9:12 ` Julien Grall
2020-09-22 20:05 ` Oleksandr
2020-09-23 18:12 ` Julien Grall
2020-09-23 20:29 ` Oleksandr
2020-09-16 9:07 ` Paul Durrant
2020-09-23 18:05 ` Julien Grall
2020-09-10 20:22 ` [PATCH V1 15/16] libxl: Introduce basic virtio-mmio support on Arm Oleksandr Tyshchenko
2020-09-10 20:22 ` [PATCH V1 16/16] [RFC] libxl: Add support for virtio-disk configuration Oleksandr Tyshchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9ebdca87-4105-c27b-635d-7a1b6d4cde82@gmail.com \
--to=olekstysh@gmail.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=jbeulich@suse.com \
--cc=julien.grall@arm.com \
--cc=julien@xen.org \
--cc=jun.nakajima@intel.com \
--cc=kevin.tian@intel.com \
--cc=oleksandr_tyshchenko@epam.com \
--cc=paul@xen.org \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=tim@xen.org \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).