Hi Manish,
On 26/02/18 06:42, Manish Jaggi wrote:
On 02/01/2018 04:24 PM, Julien Grall wrote:
Hi Manish,
On 01/02/18 08:51, Manish Jaggi wrote:
On 01/25/2018 11:37 PM, Julien Grall wrote:
Hi,
I forgot to mention one thing about the placement of
do_fixup_vgic_errata.
On 16/01/18 15:42, mja...@caviumnetworks.com wrote:
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6f6de3691..d4f0581d33 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs
*regs)
{
const union hsr hsr = { .bits = regs->hsr };
+#ifdef CONFIG_VGIC_ERRATA
+ int ret;
+
+ ret = do_fixup_vgic_errata(regs,hsr);
+ if ( !ret )
+ {
+ advance_pc(regs, hsr);
+ return;
I am fully aware that I suggested this solution and still support
that the vGIC errata should be fully separated. After all, it deals
with hardware bug and the errata will just update the LRs as the
hardware would do.
enter_hypervisor_head() will sync the LRs state to the internal vGIC
state. leave_hypervisor_head() will process pending softirq and
write/update the LRs based on the internal vGIC state.
As you rightfully did, the do_fixup_vgic_errata should be called
before syncing the LRs. However, even if you return early here, you
will still execute leave_hypervisor_tail(). This mean that pending
softirqs will be processed and potentially the vCPU rescheduled.
Because the LRs were not synced (enter_hypervisor_head()) was not
called, then the vGIC state will not out-of-date and would lead to
all sort of potential issues.
As the vGIC errata implies trapping the register such as IAR1
(reading interrupt), we want to get a fastpath for it (e.g not
trying to execute softirq...). So I think we should bypass
leave_hypervisor_tail(). I am not entirely sure how to do it nicely
thought.
How about adding a check for group1_trap enable in
leave_hypervisor_tail().
void leave_hypervisor_tail(void)
{
+if (group1_trap)
+ return;
I guess you mean the variable you introduced in patch #10. In that
case, this would be totally wrong. You only want to skip
leave_hypervisor_tail() when you are handling a ICV_* System registers.
What you want is adding a bool in the structure cpu_info to tell
whether leave_hypervisor_tail() should be skipped.
Why would it be wrong? I don't follow our point.
As I mentioned in one of my previous e-mail, leave_hypervisor_tail()
will process pending softirq and write/update the LRs based on the
internal vGIC state. Softirq are used for scheduling, handling timer,...
So unless you want to make Xen unusable on Thunder-X, you really don't
want to bypass leave_hypervisor_tail() in all the cases. Hence the
suggest of a flag in cpu_info to provide a fastpath in certain cases.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
